Conversation
- 변경된 로깅 방식에 맞춰 파일 수정 - info, warn, error, api_perf 폴더 기반 소스, 프로세서 정의
sukangpunch
requested review from
Gyuhyeok99,
Hexeong,
JAEHEE25,
lsy1307,
whqtker and
wibaek
as code owners
Walkthrough설정 파일 Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @config/side-infra/config.alloy.tftpl:
- Line 70: The template currently uses sys.env("ALLOY_ENV") which returns an
empty string if unset; update each occurrence of sys.env("ALLOY_ENV") (including
the instances used for the env label) to wrap it with
coalesce(sys.env("ALLOY_ENV"), "unknown") so the env label defaults to "unknown"
when the ALLOY_ENV variable is not set; apply this change to all occurrences
mentioned in the review.
🧹 Nitpick comments (1)
config/side-infra/config.alloy.tftpl (1)
10-39:local.file_match의level/log_type레이블과loki.process의static_labels중복 확인 필요
local.file_match블록에서level과log_type을 타겟 레이블로 정의하고 있는데, 동일한 레이블이loki.process블록의stage.static_labels에서도 다시 정의되고 있습니다 (lines 71-72, 84-85, 97-98, 110-111).두 곳 중 한 곳에서만 레이블을 정의하는 것이 명확합니다:
file_match의 레이블을 활용하려면 process에서static_labels로 재정의할 필요 없음static_labels로 일관되게 관리하려면file_match에서level/log_type제거 가능현재 구조에서 의도한 동작이 맞는지 확인해 주세요.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
config/side-infra/config.alloy.tftpl
🔇 Additional comments (2)
config/side-infra/config.alloy.tftpl (2)
41-61: LGTM!각 소스가 올바른
file_match타겟을 참조하고 해당 프로세서로 정확하게 포워딩하고 있습니다.
63-114: 설계 선택에 대한 피드백PR 설명에서 요청하신 부분에 대해:
명시적 폴더별 정의 선택: 현재 4개의 로그 타입으로는 명시적 정의가 가독성과 유지보수 측면에서 적절합니다. 다만 로그 타입이 크게 늘어난다면 반복 코드가 증가하므로, 그 시점에 동적 처리로 리팩토링을 고려하면 됩니다.
라벨링 방식 (
service,env,level,log_type): Loki 쿼리 시 일반적으로 사용되는 라벨 구조로,{service="backend", env="prod", level="error"}형태의 쿼리가 직관적입니다.log_type으로application과performance를 구분하는 것도 적절합니다.
| stage.static_labels { | ||
| values = { | ||
| service = "backend", | ||
| env = sys.env("ALLOY_ENV"), |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Grafana Alloy sys.env function behavior undefined environment variable
💡 Result:
Short answer: sys.env returns an empty string ("") when the named environment variable does not exist — not null or an error. Use coalesce or test for "" to provide a default. [1][2]
Examples:
- sys.env("DOES_NOT_EXIST") => "" . [1]
- coalesce(sys.env("DOES_NOT_EXIST"), "default") => "default". [2]
Note: if you see errors like "component 'sys.env' does not exist or is out of scope", that usually means the expression was used in a context where functions/stdlib aren’t available or referenced incorrectly — check placement/scope in your config. [3]
Sources: Grafana Alloy docs for sys.env and coalesce; community forum thread. [1][2][3]
🏁 Script executed:
# Search for the config file and examine the context around lines 70, 83, 96, 109
find . -name "config.alloy.tftpl" -o -name "*.alloy" | head -20Repository: solid-connection/solid-connection-infra
Length of output: 121
🏁 Script executed:
# Examine the config file around the mentioned lines
cat -n ./config/side-infra/config.alloy.tftpl | sed -n '50,120p'Repository: solid-connection/solid-connection-infra
Length of output: 2099
배포 환경에서 ALLOY_ENV 환경 변수 설정 확인
sys.env("ALLOY_ENV")가 설정되지 않은 경우 빈 문자열을 반환하므로, 로그의 env 레이블이 비어있게 됩니다. 배포 환경에서 ALLOY_ENV 환경 변수가 항상 설정되어 있는지 확인하세요. 필요시 coalesce(sys.env("ALLOY_ENV"), "unknown")를 사용하여 기본값을 설정할 수 있습니다.
또한 적용됨: 83-83, 96-96, 109-109
🤖 Prompt for AI Agents
In @config/side-infra/config.alloy.tftpl at line 70, The template currently uses
sys.env("ALLOY_ENV") which returns an empty string if unset; update each
occurrence of sys.env("ALLOY_ENV") (including the instances used for the env
label) to wrap it with coalesce(sys.env("ALLOY_ENV"), "unknown") so the env
label defaults to "unknown" when the ALLOY_ENV variable is not set; apply this
change to all occurrences mentioned in the review.
Hexeong
left a comment
Hexeong
left a comment
There was a problem hiding this comment.
고생많으셨습니다!
추가로 리뷰 요구사항에 대해 말씀드리자면 제 의견은 아래와 같습니다!
- 해당 파일은 애플리케이션과 달리 인프라 코드에 해당한고, 개인적으로 동적 처리보다는 기존 방식처럼 명시적 처리가 더 안전한 코드라고 생각이 드네요 ㅎㅎ 그리고 로그 종류가 적기도 하고, 종류의 변동성이 그리 높지 않다고 생각이 들기도 합니다. 그래서 저도 2번이 괜찮다고 생각합니다.
- 라벨링 방식은 찬성입니다! 특히 performance 측정을 위해 따로 로그를 빼신 것 같은데 좋다고 느껴지네요 ㅎㅎ
(추신: 코드래빗의 리뷰에 대해서는 방어로직으로 ALLOY_ENV에 대한 default 값을 coalesce로 만들 수 있으나, 다른 스크립트에서 이를 명시적으로 정의해서 호출하기에 괜찮다고 생각합니다!)
2 participants
관련 이슈
작업 내용
로그 파일 매칭 수정
각 로그 파일 매칭에 맞게 소스도 정의
각 소스에 맞게 프로세서를 정의
특이 사항
리뷰 요구사항 (선택)
현재 관리하는 로그 폴더(info, warn, error, api_perf) 별로
파일 매칭 - 소스 - 프로세스 를 각각 구현하였는데
두 생각이 들었고 2번으로 처리 했는데 의견을 여쭙고 싶습니다.
+
추가로 라벨링 방식이
다음과 같이 발생하게 되는데 해당 방식 그대로 가도 괜찮을지 궁금합니다